Refactor `template_ws` #44

j3soon · 2024-08-20T18:33:05Z

This refactor addresses several issues we encountered and discussed since the initial version of template_ws.

This also simplifies some commands, improves performance, and automate commands to make the development process more streamlined.

I would appreciate any feedback from @YuZhong-Chen and @Assume-Zhan when you have time to ensure I didn’t miss anything. If it looks good, you could submit a review by simply approve this PR, and optionally leave some comments. Thank you!

After merging this PR, we still need to:

Add some basic tests/linting to ensure all workspace styles are in sync with the template workspace (Add basic linting scripts for codebase maintenance #34)
Refactor all other workspace style (Migrate all existing workspace style to the latest template workspace style #35)

Originally, if a newly created workspace doesn't update the author info, it could lead to incorrect attribution.

…etter caching For example, common tools such as curl are seldomly changed, and could be executed before other dockerfile commands to save build time. As a side note, `useradd` is preferred over `adduser` for the following reasons: https://askubuntu.com/a/345986

…in ros base image As we will source the global ros2 env in `.bashrc`

…s` to `ros2-essentials` Related: #35

…ns and set domain ID to zero Co-authored-by: YuZhong-Chen <[email protected]> Co-authored-by: assume <[email protected]>

Fixes: #41

Fixes: #31

Fixes: #40 Co-authored-by: assume <[email protected]>

…bashrc`, Remove redundant `postCreateCommand.sh`

YuZhong-Chen · 2024-08-21T07:41:49Z

Hi, I think you missed installing Python pip in this section.

ros2-essentials/template_ws/docker/Dockerfile

Lines 40 to 43 in afdb58c

    
           # Install Python pip 
        
           RUN --mount=type=cache,target=/var/cache/apt \ 
        
               apt-get update && apt-get install -y \ 
        
               && rm -rf /var/lib/apt/lists/*

Co-authored-by: YuZhong-Chen <[email protected]>

j3soon · 2024-08-21T07:53:48Z

Hi @YuZhong-Chen , I just pushed a fix, thanks!

YuZhong-Chen · 2024-08-21T08:06:06Z

I think we can support multi-platform builds in this PR, otherwise, it will be difficult to refactor the existing workspace, such as husky_ws. However, we might not support arm64 in every workspace, we may need to add some comments in the compose file to indicate whether this Dockerfile supports arm64.

To enable arm64 architecture support in template_ws, we can add the following lines.

ros2-essentials/husky_ws/docker/Dockerfile

Lines 1 to 10 in 443c2bb

    
           # Base Image : https://hub.docker.com/r/arm64v8/ros/tags?page=1&name=humble 
        
           FROM arm64v8/ros:humble AS arm64 
        
           # Base Image : https://hub.docker.com/r/osrf/ros/tags?page=1&name=humble 
        
           FROM osrf/ros:humble-desktop-full AS amd64 
        
           # Use docker automatic platform args to select the base image. 
        
           # It may be `arm64` or `amd64` depending on the platform. 
        
           # Reference:  
        
           # - https://docs.docker.com/reference/dockerfile/#automatic-platform-args-in-the-global-scope 
        
           FROM $TARGETARCH

ros2-essentials/husky_ws/docker/Dockerfile

Lines 51 to 56 in 443c2bb

    
           RUN if [ "$TARGETARCH" = "amd64" ]; then \ 
        
                   apt-get update && apt-get install -y \ 
        
                   ros-$ROS_DISTRO-gazebo-ros-pkgs \ 
        
                   ros-$ROS_DISTRO-gazebo-ros2-control \ 
        
                   && rm -rf /var/lib/apt/lists/*; \ 
        
               fi

ros2-essentials/husky_ws/docker/compose.yaml

Lines 3 to 9 in 443c2bb

    
           build: 
        
             context: . 
        
             dockerfile: Dockerfile 
        
             # Specify the target platform to build the image. 
        
             # Reference: https://docs.docker.com/compose/compose-file/build/#platforms 
        
             # platforms: 
        
             #   - "linux/arm64"

Co-authored-by: YuZhong-Chen <[email protected]>

j3soon · 2024-08-21T08:24:20Z

@YuZhong-Chen Nice suggestion! just added this.

YuZhong-Chen · 2024-08-21T08:42:55Z

I haven't used Docker build cache before, so I'm currently learning about it step-by-step from the official documentation. I believe our current Dockerfile is still missing some commands.

Here’s an example from BuildKit:

# syntax=docker/dockerfile:1
FROM ubuntu
RUN rm -f /etc/apt/apt.conf.d/docker-clean; echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache
RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
  --mount=type=cache,target=/var/lib/apt,sharing=locked \
  apt update && apt-get --no-install-recommends install -y gcc

Based on the example above, we should:

Remove docker-clean and add some commands in keep-cache. Ref1 Ref2.
Add sharing=private to prevent errors when building multiple Docker images in parallel. Ref

I'm currently not sure if we need to mount /var/lib/apt, because we have encountered issues before where this cache prevented us from updating apt lib lists. I prefer sticking with the existing approach: directly deleting it and regenerating it each time it's used.

If you want to learn more, I think this website explains it very clearly.

YuZhong-Chen · 2024-08-21T09:25:40Z

I'm sorry, I didn't clarify the previous example well. You should mount the cache here in commit f8e86d9

By the way, we could add some examples in the TODO section to remind users to use the cache.

# TODO: Add more commands here
# For example, to install additional packages, uncomment the following lines and add the package names
# RUN --mount=type=cache,target=/var/cache/apt \
#     apt-get update && apt-get install -y \
#     $OTHER_PACKAGES \
#     && rm -rf /var/lib/apt/lists/*

However, mounting the cache might involve changes, we can wait until the previous comment is resolved.

… sharing to allow concurrent builds We only cache `/var/cache/apt` and do not cache `/var/lib/apt`, since the main purpose of this cache mount is to speed up package downloads. Caching `/var/lib/apt` might offer a slight speed improvement, but its potential drawbacks could outweigh the benefits. We use `sharing=private` instead of `sharing=locked`, since if we are building multiple images simultaneously, locking the cache may make the builds slower. In addition, the `private` sharing mode can still share caches across different image builds when the cache isn't accessed simultaneously. For more information, see the references in comments or the discussion in the PR: #44 (comment) Co-authored-by: YuZhong-Chen <[email protected]>

j3soon · 2024-08-21T14:06:14Z

@YuZhong-Chen Thanks for the info! I'm also new to this feature and learned a lot by going through the detailed references you provided. I agree with your suggestions on this issue, and just pushed a fix accordingly.

Remove the Gazebo cache since we use Docker volumes instead of cache in each workspace.

…ite errors. Sometimes we might use packages that are no longer maintained. Although these packages can compile normally, `rosdep` might report errors because the `package.xml` is not updated. Using this flag can help avoid this issue. If a package is missing, it will still be displayed in the terminal, so you don’t need to worry about missing error messages.

YuZhong-Chen

I have added some commits to fix the issues I observed. If these commits are fine, I believe the current changes can be merged into the master branch.

By the way, should we rename the master branch to main? Although this change won’t affect anything, I prefer the main naming convention, and the official default branch name has been updated. You can refer to this link for more information.

j3soon · 2024-08-21T17:06:46Z

Thanks for reviewing this PR. I'll go ahead and merge it.

I agree with you and just renamed the default branch from master to main. Thanks for the reminder!

Ref: #44 (review)

j3soon and others added 11 commits August 20, 2024 15:19

feat(template_ws): Remove docker image author info

61cee56

Originally, if a newly created workspace doesn't update the author info, it could lead to incorrect attribution.

feat(template_ws): Add more common tools

980f254

feat(template_ws): Reset redundant entrypoint (/ros_entrypoint.sh) …

658971f

…in ros base image As we will source the global ros2 env in `.bashrc`

refactor(template_ws): Replace all old names from `ros2-agv-essential…

93e88b9

…s` to `ros2-essentials` Related: #35

refactor(template_ws): Clean up and re-order compose file

9d43032

feat(template_ws): Change default to allow non-localhost communicatio…

680d12d

…ns and set domain ID to zero Co-authored-by: YuZhong-Chen <[email protected]> Co-authored-by: assume <[email protected]>

perf(template_ws): Speed up local docker build with cache mounts

f439bf4

Fixes: #41

refactor(template_ws): Change gazebo cache directory to volume mount

79ac41c

Fixes: #31

feat(template_ws): Auto compile in .bashrc

2b509a7

Fixes: #40 Co-authored-by: assume <[email protected]>

feat(template_ws): Perform apt-get update and rosdep update in `.…

afdb58c

…bashrc`, Remove redundant `postCreateCommand.sh`

fix(template_ws): Add back the missing python3-pip

b1bd29f

Co-authored-by: YuZhong-Chen <[email protected]>

feat(template_ws): Add multi-platform builds support

f8e86d9

Co-authored-by: YuZhong-Chen <[email protected]>

j3soon requested a review from YuZhong-Chen August 21, 2024 14:06

YuZhong-Chen added 3 commits August 21, 2024 23:08

fix(template_ws): Update the .gitignore

39f5981

Remove the Gazebo cache since we use Docker volumes instead of cache in each workspace.

docs(template_ws): Update README.md

4852dfe

YuZhong-Chen approved these changes Aug 21, 2024

View reviewed changes

j3soon merged commit aae39be into main Aug 21, 2024
1 check passed

j3soon deleted the refactor/template-ws branch August 21, 2024 17:06

j3soon added a commit that referenced this pull request Aug 21, 2024

refactor: Change default branch from master to main and add tests

2b06fd5

Ref: #44 (review)

j3soon added a commit that referenced this pull request Aug 21, 2024

refactor: Change default branch from master to main and add tests

7bf287e

Ref: #44 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `template_ws` #44

Refactor `template_ws` #44

j3soon commented Aug 20, 2024 •

edited

Loading

YuZhong-Chen commented Aug 21, 2024

j3soon commented Aug 21, 2024

YuZhong-Chen commented Aug 21, 2024

j3soon commented Aug 21, 2024

YuZhong-Chen commented Aug 21, 2024

YuZhong-Chen commented Aug 21, 2024 •

edited

Loading

j3soon commented Aug 21, 2024

YuZhong-Chen left a comment

j3soon commented Aug 21, 2024

Refactor template_ws #44

Refactor template_ws #44

Conversation

j3soon commented Aug 20, 2024 • edited Loading

YuZhong-Chen commented Aug 21, 2024

j3soon commented Aug 21, 2024

YuZhong-Chen commented Aug 21, 2024

j3soon commented Aug 21, 2024

YuZhong-Chen commented Aug 21, 2024

YuZhong-Chen commented Aug 21, 2024 • edited Loading

j3soon commented Aug 21, 2024

YuZhong-Chen left a comment

Choose a reason for hiding this comment

j3soon commented Aug 21, 2024

Refactor `template_ws` #44

Refactor `template_ws` #44

j3soon commented Aug 20, 2024 •

edited

Loading

YuZhong-Chen commented Aug 21, 2024 •

edited

Loading